Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated hydrators #933

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Generated hydrators #933

wants to merge 1 commit into from

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented May 1, 2018

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? yes
Deprecations? no
Tests pass? no
Fixed tickets #904
License Apache-2.0

tried to implement #904, but for now looks slower...must be doing something wrong

@goetas
Copy link
Collaborator Author

goetas commented May 1, 2018

@Majkl578 What do you think?

@goetas
Copy link
Collaborator Author

goetas commented May 2, 2018

I have checked the approach used internally by ocraminus, in reality used Closure::bind .... so it is not faster that what we have now... and is exactly Closure::bind 😄
Not sure if we makes sense to continue this experiment.

@Majkl578
Copy link
Contributor

Majkl578 commented May 2, 2018

in reality used Closure::bind

Yes, that is true. But he does that at once for whole object, not per-property. Calling closure per property (imagine 30 properties per object) will be slower than calling it once for all :)

@goetas
Copy link
Collaborator Author

goetas commented May 2, 2018

the version in this PR is 10% slower that the current master... 😕 .... still to figure out why.

@Majkl578
Copy link
Contributor

Majkl578 commented May 2, 2018

As noted on Slack:

ad your generated hydrator: you don't use production ready configuration (always generate PHP code + eval() it instead),

$this->gen[$class] = new $hydratorClass();
}

return $this->gen[$class]->extract($object);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578 generators are generated and cached, so is "production" ready after the warmup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code inside the "if" is never executed during the benchmark, so should not make it slower

@goetas goetas added this to the v3.0 milestone May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants